Don't assemble non-env/bound candidates if projection is rigid #139762

compiler-errors · 2025-04-13T20:02:56Z

Putting this up for an initial review, it's still missing comments, clean-up, and possibly a tweak to deal with ambiguities in the BestObligation folder.

This PR fixes rust-lang/trait-system-refactor-initiative#173. Specifically, we're creating an unnecessary query cycle in normalization by assembling an impl candidate even if we know later on during merge_candidates that we'll be filtering out that impl candidate.

This PR adjusts the merge_candidates to assemble only env/bound candidates if we have TraitGoalProvenVia::ParamEnv | TraitGoalProvenVia::AliasBound.

I'll leave some thoughts/comments in the code.

r? lcnr

rustbot · 2025-04-13T20:03:02Z

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

compiler/rustc_next_trait_solver/src/solve/assembly/mod.rs

compiler/rustc_trait_selection/src/solve/fulfill/derive_errors.rs

lcnr · 2025-04-14T09:48:21Z

This PR has two separate effects

we avoid fetching the type of impl associated items if they are shadowed anyways
- avoid the query cycle in leptos, and generally avoids an unnecessary query dependency
we avoid reassembling impl candidates if stuff is proven via where-bound, likely positively impacting perf

I feel 🤷 about the first case. I think we should fix #139788 regardless as using RPITITs recursively shouldn't cycle even if they are actually needed. It does match the behavior of the old solver so it feels generally reasonable to do so.

The second one is a bit worrying. I would be opposed/far more hesitant if it were to do the same when assembling trait goals as that definitely has an exponential performance impact (e.g. it would fix rust-lang/trait-system-refactor-initiative#109). I want to be very careful here as I expect us to consider impl candidates in more cases in the future and ideally doing so won't introduce new hangs :<

I expect that in theory we may have exponential performance differences depending on whether or not we check impl item bounds. I don't know if they are likely enough to actually cause issues for us in the future. I am still a bit on the fence about this PR, but generally in favor I think.

compiler-errors · 2025-04-14T10:14:08Z

I would be opposed/far more hesitant if it were to do the same when assembling trait goals as that definitely has an exponential performance impact (e.g. it would fix rust-lang/trait-system-refactor-initiative#109).

Well part of the point of this approach is we're already doing all the work of proving the trait goal in order to even compute the TraitGoalProvenVia, so my vibe is that all of the additional extra possibly exponential computational work here that we're winnowing out today would be due to GAT bounds.

Totally unrelated side-note that it would be nice if query cycles were recoverable, lol.

bors · 2025-04-15T12:20:07Z

☔ The latest upstream changes (presumably #139845) made this pull request unmergeable. Please resolve the merge conflicts.

lcnr · 2025-04-15T12:32:40Z

so my vibe is that all of the additional extra possibly exponential computational work here that we're winnowing out today would be due to GAT bounds.

yeah, exactly, the only hangs we'd be avoiding this way are due to GAT bounds. I guess this seems acceptable to me, so yeah. Let's go ahead with this change

lcnr

an alternative structure would be to change assemble_and_evaluate_candidates to also take

enum AssembledCandidates {
   All,
   /// Only assemble candidates from the environment,
   /// ignoring impls. We only look for `ParamEnv` and
   /// `AliasBound` candidates.
   FromEnv,
}

and then put assemble_impl_candidates behind an if/match 🤔

I feel like splitting assembly into multiple functions is making this a bit harder to reason about.

thoughts?

compiler-errors · 2025-04-17T18:58:46Z

Yeah, that would work too.

compiler/rustc_next_trait_solver/src/solve/assembly/mod.rs

lcnr · 2025-04-17T19:14:55Z

compiler/rustc_next_trait_solver/src/solve/assembly/mod.rs

        inject_normalize_to_rigid_candidate: impl FnOnce(&mut EvalCtxt<'_, D>) -> QueryResult<I>,
    ) -> QueryResult<I> {
        let Some(proven_via) = proven_via else {
            // We don't care about overflow. If proving the trait goal overflowed, then
            // it's enough to report an overflow error for that, we don't also have to
            // overflow during normalization.
-            return Ok(self.make_ambiguous_response_no_constraints(MaybeCause::Ambiguity));
+            return Ok(self.forced_ambiguity(MaybeCause::Ambiguity)?.result);


any case where make_ambiguous_response_no_constraints was causing issues? or just general consistency. Also isn't this just return self.forced_ambiguity()? :<

any case where make_ambiguous_response_no_constraints was causing issues?

make_ambiguous_response_no_constraints was causing us not to register a candidate kind, which is the reason why I had commented out that assertion in the BestObligation folder above.

#139762 (comment)

I prefer just actually assembling a built-in candidate for ambiguity here, but if you care a lot about this then we could just remove the assertion I linked above, but it does mean unnecessarily stepping into the "search for failures due to no candidates assembled" code in the best obligation folder.

lso isn't this just return self.forced_ambiguity()? :<

Nope, since we need to unwrap the candidate and return its query response.

i.e. return Ok(x?.field) is not return x.

return x.map(|c| c.result) then?

[DO NOT MERGE] bootstrap with `-Znext-solver=globally` A revival of rust-lang#124812. Current status: ~~`./x.py b --stage 2` passes 🎉~~ `try` builds succeed 🎉 🎉 🎉 [top 100 most downloaded crates on crates.io compile](rust-lang#133502 (comment)) [first perf run](rust-lang#133502 (comment)) 👻 ### in-flight changes - rust-lang#124852, unsure whether I actually want to land this PR for now - rust-lang#139587 - https://github.com/lcnr/rust/tree/opaque-type-method-call - rust-lang#138845 - rust-lang#139762 - double the available recursion depth in the new solver ☠️ r? `@ghost`

[DO NOT MERGE] bootstrap with `-Znext-solver=globally` A revival of rust-lang#124812. Current status: ~~`./x.py b --stage 2` passes 🎉~~ `try` builds succeed 🎉 🎉 🎉 ~~[top 100 most downloaded crates on crates.io compile](rust-lang#133502 (comment) [top 1000 most downloaded crates on crates.io compile](rust-lang#133502 (comment)) [first perf run](rust-lang#133502 (comment)) 👻 ### in-flight changes - rust-lang#124852, unsure whether I actually want to land this PR for now - rust-lang#139587 - https://github.com/lcnr/rust/tree/opaque-type-method-call - rust-lang#138845 - rust-lang#139762 - double the available recursion depth in the new solver ☠️ r? `@ghost`

lcnr · 2025-04-19T11:24:54Z

@bors r+ rollup

bors · 2025-04-19T11:24:57Z

📌 Commit e882ff4 has been approved by lcnr

It is now in the queue for this repository.

…enton Rollup of 8 pull requests Successful merges: - rust-lang#138934 (support config extensions) - rust-lang#139091 (Rewrite on_unimplemented format string parser.) - rust-lang#139753 (Make `#[naked]` an unsafe attribute) - rust-lang#139762 (Don't assemble non-env/bound candidates if projection is rigid) - rust-lang#139834 (Don't canonicalize crate paths) - rust-lang#139868 (Move `pal::env` to `std::sys::env_consts`) - rust-lang#139978 (Add citool command for generating a test dashboard) - rust-lang#139995 (Clean UI tests 4 of n) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#139762 - compiler-errors:non-env, r=lcnr Don't assemble non-env/bound candidates if projection is rigid Putting this up for an initial review, it's still missing comments, clean-up, and possibly a tweak to deal with ambiguities in the `BestObligation` folder. This PR fixes rust-lang/trait-system-refactor-initiative#173. Specifically, we're creating an unnecessary query cycle in normalization by assembling an *impl candidate* even if we know later on during `merge_candidates` that we'll be filtering out that impl candidate. This PR adjusts the `merge_candidates` to assemble *only* env/bound candidates if we have `TraitGoalProvenVia::ParamEnv | TraitGoalProvenVia::AliasBound`. I'll leave some thoughts/comments in the code. r? lcnr

…enton Rollup of 8 pull requests Successful merges: - rust-lang#138934 (support config extensions) - rust-lang#139091 (Rewrite on_unimplemented format string parser.) - rust-lang#139753 (Make `#[naked]` an unsafe attribute) - rust-lang#139762 (Don't assemble non-env/bound candidates if projection is rigid) - rust-lang#139834 (Don't canonicalize crate paths) - rust-lang#139868 (Move `pal::env` to `std::sys::env_consts`) - rust-lang#139978 (Add citool command for generating a test dashboard) - rust-lang#139995 (Clean UI tests 4 of n) r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned lcnr Apr 13, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Apr 13, 2025

compiler-errors commented Apr 13, 2025

View reviewed changes

compiler-errors force-pushed the non-env branch from 55069b6 to 513ace6 Compare April 16, 2025 18:53

lcnr mentioned this pull request Apr 17, 2025

rustdoc: use the next solver for blanket impl synthesis #125907

Open

lcnr reviewed Apr 17, 2025

View reviewed changes

compiler-errors force-pushed the non-env branch from 513ace6 to f7098be Compare April 17, 2025 19:07

compiler-errors commented Apr 17, 2025

View reviewed changes

compiler/rustc_next_trait_solver/src/solve/assembly/mod.rs Show resolved Hide resolved

lcnr reviewed Apr 17, 2025

View reviewed changes

compiler/rustc_next_trait_solver/src/solve/assembly/mod.rs Outdated Show resolved Hide resolved

lcnr reviewed Apr 17, 2025

View reviewed changes

compiler-errors force-pushed the non-env branch from f7098be to 39a8eca Compare April 17, 2025 19:21

lcnr mentioned this pull request Apr 17, 2025

[DO NOT MERGE] bootstrap with -Znext-solver=globally #133502

Draft

Don't assemble non-env/bound candidates if projection is rigid

e882ff4

compiler-errors force-pushed the non-env branch from 39a8eca to e882ff4 Compare April 18, 2025 01:44

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 19, 2025

ChrisDenton mentioned this pull request Apr 19, 2025

Rollup of 8 pull requests #140043

Merged

bors merged commit 688478f into rust-lang:master Apr 20, 2025
6 checks passed

rustbot added this to the 1.88.0 milestone Apr 20, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't assemble non-env/bound candidates if projection is rigid #139762

Don't assemble non-env/bound candidates if projection is rigid #139762

compiler-errors commented Apr 13, 2025 •

edited

Loading

rustbot commented Apr 13, 2025

lcnr commented Apr 14, 2025

compiler-errors commented Apr 14, 2025 •

edited

Loading

bors commented Apr 15, 2025

lcnr commented Apr 15, 2025

lcnr left a comment

compiler-errors commented Apr 17, 2025

lcnr Apr 17, 2025

compiler-errors Apr 17, 2025

lcnr Apr 17, 2025

lcnr commented Apr 19, 2025

bors commented Apr 19, 2025

Don't assemble non-env/bound candidates if projection is rigid #139762

Don't assemble non-env/bound candidates if projection is rigid #139762

Conversation

compiler-errors commented Apr 13, 2025 • edited Loading

rustbot commented Apr 13, 2025

lcnr commented Apr 14, 2025

compiler-errors commented Apr 14, 2025 • edited Loading

bors commented Apr 15, 2025

lcnr commented Apr 15, 2025

lcnr left a comment

Choose a reason for hiding this comment

compiler-errors commented Apr 17, 2025

lcnr Apr 17, 2025

Choose a reason for hiding this comment

compiler-errors Apr 17, 2025

Choose a reason for hiding this comment

lcnr Apr 17, 2025

Choose a reason for hiding this comment

lcnr commented Apr 19, 2025

bors commented Apr 19, 2025

compiler-errors commented Apr 13, 2025 •

edited

Loading

compiler-errors commented Apr 14, 2025 •

edited

Loading